Skip to content

Refactor codegen backends in bootstrap #144787

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Kobzol
Copy link
Member

@Kobzol Kobzol commented Aug 1, 2025

This PR refactors the codegen backend steps, in preparation to make more progress on the integration of the GCC codegen backend in bootstrap. It does several things:

  1. Splits the CodegenBackend step into two, one for clif and another one for gcc. Even though their code is mostly similar, that's IMO mostly fake similarity, and they do (or will) ultimately require different handling. This was already visible in the requirement of building GCC for cg_gcc, of course.
  2. It is now possible to build both backends (and dist cranelift) even if they are not specified in rust.codegen-backends. It was quite weird that it wasn't possible to even invoke the corresponding codegen backend if the backend wasn't specified in that array, as that array should ideally only change defaults (see later below).
  3. Changes the path specification of these steps to an alias. In other words, instead of compiler/rustc_codegen_cranelift, the step is now built only using rustc_codegen_cranelift or cg_clif. This is done to avoid an annoying clash with x build compiler, which would otherwise build both codegen backends after the 2) change.
  4. Made the copying of codegen backend artifacts more explicit, in particular in the Assemble step.
  5. Codifies the semantics of rust.codegen-backends, which now only affects the defaults of whether a codegen backend will be included in rustc's sysroot and whether it will be disted in x dist by default. We can change the behavior later, e.g. to dist cranelift by default in x dist once it becomes stabilized. Currently I left the existing behavior that we use on CI, I just tried to document it better.

I don't think that this requires a change tracker entry, because the defaults should work the same as before. It is just now possible to do x build/dist rustc_codegen_cranelift even if CLIF is not in the codegen-backends array. It is no longer possible to do ./x build compiler/rustc_codegen_cranelift though, not sure if that requires a change tracker entry.

There is one thing that I didn't touch yet, and that is the fact that rust.codegen-backends not only affects the default behavior of x dist w.r.t. Cranelift, but also of x test. In other words, x test rustc_codegen_cranelift still does not hing if cranelift isn't in rust.codegen-backends. I plan to take a look at this once I get to refactoring the test steps.

r? @jieyouxu

@rustbot
Copy link
Collaborator

rustbot commented Aug 1, 2025

jieyouxu is currently at their maximum review capacity.
They may take a while to respond.

@Kobzol Kobzol changed the title Refactor codegen backends Refactor codegen backends in bootstrap Aug 1, 2025
@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Aug 1, 2025
@rustbot
Copy link
Collaborator

rustbot commented Aug 1, 2025

This PR modifies src/bootstrap/src/core/config.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

This PR modifies bootstrap.example.toml.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

@rust-log-analyzer

This comment has been minimized.

@jieyouxu
Copy link
Member

jieyouxu commented Aug 5, 2025

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 5, 2025
@Kobzol Kobzol force-pushed the codegen-backend-restructure branch from 588f96f to 0a3b2d7 Compare August 5, 2025 14:49
@Kobzol
Copy link
Member Author

Kobzol commented Aug 5, 2025

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 5, 2025
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I have some nits/questions

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 6, 2025
@Kobzol
Copy link
Member Author

Kobzol commented Aug 6, 2025

Pushed review changes.

Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, two nits because I only just noticed

@Kobzol Kobzol force-pushed the codegen-backend-restructure branch from 4235812 to e89c64f Compare August 6, 2025 13:35
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@jieyouxu
Copy link
Member

jieyouxu commented Aug 6, 2025

@bors r+ rollup=never (build system changes, just in case)

@bors
Copy link
Collaborator

bors commented Aug 6, 2025

📌 Commit e89c64f has been approved by jieyouxu

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 6, 2025
@Zalathar
Copy link
Contributor

Zalathar commented Aug 7, 2025

Let's run a never PR while I investigate the recent rollup failure.

@bors p=1

@Zalathar
Copy link
Contributor

Zalathar commented Aug 7, 2025

Undoing my previous priority bump, since the immediate scheduling concern was solved.

@bors p=0

@Kobzol Kobzol force-pushed the codegen-backend-restructure branch from e89c64f to 1a2ee80 Compare August 8, 2025 09:37
@Kobzol
Copy link
Member Author

Kobzol commented Aug 8, 2025

Rebased and blessed snapshot tests.

@bors r=jieyouxu

@bors
Copy link
Collaborator

bors commented Aug 8, 2025

📌 Commit 1a2ee80 has been approved by jieyouxu

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 8, 2025
@bors
Copy link
Collaborator

bors commented Aug 8, 2025

⌛ Testing commit 1a2ee80 with merge e723c3c...

bors added a commit that referenced this pull request Aug 8, 2025
Refactor codegen backends in bootstrap

This PR refactors the codegen backend steps, in preparation to make more progress on the integration of the GCC codegen backend in bootstrap. It does several things:

1) Splits the `CodegenBackend` step into two, one for clif and another one for gcc. Even though their code is mostly similar, that's IMO mostly fake similarity, and they do (or will) ultimately require different handling. This was already visible in the requirement of building GCC for cg_gcc, of course.
2) It is now possible to build both backends (and dist cranelift) even if they are not specified in `rust.codegen-backends`. It was quite weird that it wasn't possible to even invoke the corresponding codegen backend if the backend wasn't specified in that array, as that array should ideally only change defaults (see later below).
3) Changes the path specification of these steps to an alias. In other words, instead of `compiler/rustc_codegen_cranelift`, the step is now built only using `rustc_codegen_cranelift` or `cg_clif`. This is done to avoid an annoying clash with `x build compiler`, which would otherwise build both codegen backends after the 2) change.
4) Made the copying of codegen backend artifacts more explicit, in particular in the `Assemble` step.
5) Codifies the semantics of `rust.codegen-backends`, which now only affects the defaults of whether a codegen backend will be included in rustc's sysroot and whether it will be disted in `x dist` by default. We can change the behavior later, e.g. to dist cranelift by default in `x dist` once it becomes stabilized. Currently I left the existing behavior that we use on CI, I just tried to document it better.

I don't think that this requires a change tracker entry, because the defaults should work the same as before. It is just now possible to do `x build/dist rustc_codegen_cranelift` even if CLIF is not in the `codegen-backends` array. It is no longer possible to do ` ./x build compiler/rustc_codegen_cranelift` though, not sure if that requires a change tracker entry.

There is one thing that I didn't touch yet, and that is the fact that `rust.codegen-backends` not only affects the default behavior of `x dist` w.r.t. Cranelift, but also of `x test`. In other words, `x test rustc_codegen_cranelift` still does not hing if cranelift isn't in `rust.codegen-backends`. I plan to take a look at this once I get to refactoring the test steps.

r? `@jieyouxu`
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-aux failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)
failures:

---- f32::test_gamma stdout ----

thread 'f32::test_gamma' (194438) panicked at library/std/tests/floats/f32.rs:198:5:
2.0000012 is not approximately equal to 2.0 (threshold 1e-6, difference 1.1920929e-6)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
note: in Miri, you may have to set `MIRIFLAGS=-Zmiri-env-forward=RUST_BACKTRACE` for the environment variable to have an effect


failures:
    f32::test_gamma

test result: FAILED. 28 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 4.81s

error: test failed, to rerun pass `-p std --test floats`
Build completed unsuccessfully in 0:04:39
make: *** [Makefile:58: check-aux] Error 1
  local time: Fri Aug  8 17:07:15 UTC 2025
  network time: Fri, 08 Aug 2025 17:07:15 GMT
##[error]Process completed with exit code 2.
Post job cleanup.

@bors
Copy link
Collaborator

bors commented Aug 8, 2025

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 8, 2025
@lqd
Copy link
Member

lqd commented Aug 8, 2025

I knew it, we may need to close the tree

@Kobzol
Copy link
Member Author

Kobzol commented Aug 8, 2025

@bors treeclosed=100 A test is failing on CI.

I guess we just comment out the test for now?

@Kobzol
Copy link
Member Author

Kobzol commented Aug 8, 2025

#145116 landed, so reopening the tree.

@bors treeclosed-

@Kobzol
Copy link
Member Author

Kobzol commented Aug 8, 2025

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants